Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

+Debug for CoordinateType #602

Merged
merged 12 commits into from
Jan 13, 2021
Merged

+Debug for CoordinateType #602

merged 12 commits into from
Jan 13, 2021

Conversation

michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented Jan 11, 2021

  • I agree to follow the project's code of conduct.
  • [TODO] I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This is intended to address #597

I took a broader approach, and wonder if people are interested in it. The tldr is:

geo-types

  • deprecate CoordinateType; (now includes std::fmt::Debug)
  • Add trait CoordNum: CoordinateType;
  • Add trait CoordFloat: CoordNum + num_traits::Float

geo

Add trait GeoNum: CoordNum + HasKernel
Add trait GeoFloat: CoordFloat + HasKernel

downstream adoption

If your crate interops with geo-types, you may need to make changes like this:

pub fn foo<T>(line_string: &geo_types::LineString<T>)
where
-    T: num_traits::Float,
+    T: geo_types::CoordFloat,
{

or this (this one is actually only a deprecation, so you could leave it as is for now, but eventually you'll need to make the change):

pub fn foo<T>(line_string: &geo_types::LineString<T>)
where
-    T: geo_types::CoordinateType
+    T: geo_types::CoordNum
{

alternative considered

If I went with the minimal approach to adding Debug trait, changes would instead need to look like this:

This one, for non-floats, could remain untouched:

pub fn foo<T>(line_string: &geo_types::LineString<T>)
where
    T: geo_types::CoordinateType
{

But anything restricted to floats would need to explicitly include Debug

pub fn foo<T>(line_string: &geo_types::LineString<T>)
where
-    T: num_traits::Float,
+    T: num_traits::Float + std::fmt::Debug,
{

motivation:

I initially completed #597 with the most conservative approach, achieved in the first two commits. The first simply adds Debug to CoordinateType and the second fixes the resulting compiler errors.

This is a breaking change for anyone constraining generic implementations to just num_traits::Float, e.g. like geojson crate does (fix here: https://github.com/georust/geojson/compare/mkirk/geo-types-0.7?expand=1), and polylabel (fix here: https://github.com/urschrei/polylabel-rs/compare/master...michaelkirk:mkirk/geo-types-0.7?expand=1)

Compare this to a crate like geo-booleanop, for which things "just work" because they targeted CoordinateType.

This got me thinking that, similar to #577 (yet unreleased), if we want people to write code targeting geo-types in a generic way, it might be helpful to make it clearer and easier how to do so.

So, I've added a CoordFloat and a CoordNum which is basically just a rename of CoordinateType, and applied them liberally within our own code base. I'm hoping this would drive others to reach for these traits rather than specific num_traits when implementing code intended for generic geo-types interop.

For crates like geo-booleanop, which targeted CoordinateType, things "just work" after the update, but they'll see a deprecation notice like:

$ cargo build
warning: use of deprecated trait `geo_types::CoordinateType`: use `CoordFloat` or `CoordNum` instead
 --> lib/src/boolean/helper.rs:2:29
  |
2 | use geo_types::{Coordinate, CoordinateType};
  |                             ^^^^^^^^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default

Which I think is acceptably clear.

It's a large number of lines changed change overall, I know! But it's mostly mechanical, and I thought, since we're breaking geo-types, maybe this is a good time to bite the bullet and avoid some future pain.

If it's too controversial, as I said, the minimal amount of change we need to enable Debug for CoordinateType is the first three commits (through and including "address fallout of "Add Debug to CoordinateType"). It's still a pretty big change with just that, and note that this will still break the crates I mentioned. I intend to fix the ones I know about, but there's a long tail of crates, which is why I hate breaking geo-types.

In the few that I've checked it's been quick to adapt to the change, but it might be worth announcing and letting sit open for a while in case downstream geo-types users want to weigh in.

Not sure why this was only enabled for Float
Lots of fallout that needs to be addressed
Rather than breaking CoordinateType, we add a Debug to a new CoordNum
type and deprecate the CoordinateType trait.

This is *less* breaking, than the previous approach, but still breaks
some API's which now require + Debug.

I prefer this to adding Debug directly to CoordinateType because:
1. breaks for less people
2. adds deprecation notice to document inline how to upgrade
3. incorporates a desirable rename
I think this will be clearer than "HasKernel".

We still often use CoordNum (things which *don't* implement HasKernel).

Do we actually care to make that distinction? Seems like it complicates
things a bit for a usecase I'm not sure exists in the wild.
@michaelkirk michaelkirk force-pushed the mkirk/debug-coordinate-type branch from 72a55b7 to 48e9fae Compare January 11, 2021 22:36
Copy link
Member

@frewsxcv frewsxcv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love this!

/// For algorithms which only make sense for floating point, like area or length calculations,
/// see [CoordFloat](trait.CoordFloat.html).
#[allow(deprecated)]
pub trait CoordNum: CoordinateType + Debug {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! I was thinking recently about renaming CoordinateType recently to something like CoordScalar, but I wasn't sure how accurate that is, or if it makes sense since "scalar" is usually used in vectors. But I like CoordNum! Seems much simpler

@frewsxcv
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 13, 2021

Build succeeded:

@bors bors bot merged commit 2c8882e into master Jan 13, 2021
@michaelkirk michaelkirk deleted the mkirk/debug-coordinate-type branch February 10, 2021 18:04
@nyurik
Copy link
Member

nyurik commented Mar 15, 2022

@michaelkirk hi, what's the status of the deprecations related to this PR? What kind of changes should be done to this and other repos that haven't been done yet? This may simplify adding extra dimensions if I don't have to support deprecated features. Thx!

@michaelkirk
Copy link
Member Author

The deprecated code here is for using CoordinateType as an alias of CoordNum. I don't immediately understand how this could substantially complicate things for your implementation.

But in general, I'd advocate for deleting deprecated code the next time we have a breaking release. No breaking release is scheduled because we haven't had sufficient reason for breaking geo-types.

We want to break geo-types rarely, and preferably accrue any less important breaking changes so we can break all at once (so that we hopefully won't have reason to break again right away).

@nyurik
Copy link
Member

nyurik commented Mar 15, 2022

@michaelkirk thx, makes sense about the breaking changes. There are a number of deprecated features, like the ones in

impl<T: CoordNum> Geometry<T> {
-- so with my dimensional support, I wouldn't want to try to upgrade them only to delete them shortly after. Plus I am pretty sure the multi-dimension support will be a breaking change, so for simplicity it would make it easier to delete deprecated features as part of that. Perhaps I should start an "official" branch in the main geo repo for multidimensional efforts, and do all the work there that would include the deprecated stuff.

WRT CoordinateType being an alias of CoordNum -- so I guess I should just delete the CoordinateType from the repo as part of this effort for simplicity sake (and to keep all breaking changes together).

What do you think?

@nyurik
Copy link
Member

nyurik commented Mar 15, 2022

P.S. I created a #772 that does the deprecation, and proposes a path forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants